Refactor SetMethod(...) #566
Conversation
|
Clever use of the dummy argument to distinguish types while preserving normal type matching. This seems like an elegant solution. |
| tpl->SetClassName(fn_name); | ||
| // Note(@agnat): Pass an empty T* as discriminator. See note on | ||
| // SetMethodAux(...) above | ||
| imp::SetMethodAux(recv, fn_name, tpl, (T*)0); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
It's going to be interesting to see if VS 201[023] accept this. |
|
Visual Studio 2013 has no problems. https://ci.appveyor.com/project/RodVagg/nan/build/1157 |
Latest v8 disallows setting non-primitive values on v8::Template and derived types. So we no longer can assign v8::Functions directly since it results in a v8 runtime warning reported in #558 [0]. Instead we assign v8::FunctionTemplates. The initial approach of using a bunch of overloads does not cover all cases as was reported in #564 [1] which also discusses the technical reason behind this regression. The new approach employs two auxiliary functions that take an additional argument to discriminate v8::Templates from non-templates. It deals with derived types correctly since the discriminating argument (a pointer) is subject to "normal" conversion rules. The entry point SetMethod(...) "peels off" the handle type using a template-template parameter. This gives access to the "inner type" T. It also deals nicely with the Local<> vs. Handle<> schism which is still an issue with historic node/v8 versions. It also adds tests for both the runtime warning in #558 and the regression from #564. Finally, it tunes "npm test" to show v8 runtime deprecation warnings. This closes #564. [0] #558 [1] #564
|
Looking good. I like it. Much cleaner than unrolling everything. I would think this should work on older VS compilers than that in 2013, but have no easy way of testing it. However, since this whole workaround is only strictly necessary for Node 6 and later, and already Node 4 requires a C++11 compiler, it could even be ifdefed around in case it turns out that it break some old compiler for some old version of Node and someone cared enough about it. @bnoordhuis You'll want something like this for Node core as well, since |
agnat
force-pushed
the
fix/SetMethod_dispatch
branch
from
a9b3207
to
8aad6c8
Compare
|
@kkoopa You could try to add the |
|
I'm confident that it will just work... right down to historic, single digit versions of VS. ;) I didn't invent it. I've seen it somewhere... in a high-quality code base... probably boost. If it really does act up I'll come up with something (else). TIMTOWTDI. I removed my beautiful application of a C cast in C++, ironed the diff to remove some wrinkles like @bnoordhuis suggested, squashed the thing and added a proper commit message. PTAL. |
|
LGTM. Unless someone raises a valid objection, I'll merge this and push out a new version by tomorrow. |
As discussed in #564 here is a first stab at refactoring
SetMethod(...). @kkoopa, @bnoordhuis PTAL and let me know what you think.